Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

revert(gazelle): revert #1895 as it is causing issues cross-building the plugin #1931

Closed
wants to merge 1 commit into from
Closed

revert(gazelle): revert #1895 as it is causing issues cross-building the plugin #1931

wants to merge 1 commit into from

Conversation

aignas
Copy link
Collaborator

@aignas aignas commented May 30, 2024 •
edited
Loading

This reverts commit 7fc7962.

#1895 has introduced a way to parse Python files without the need for
Python interpreter by leveraging the tree-sitter bindings for go.
However, those bindings introduced a C dependency and as a result it
was breaking the cross-compilation of the gazelle plugin.

The alternatives to revert are:

  • Have 2 code paths, one using interpreter and one using the
    tree-sitter, which would benefit users who don't need
    cross-compilation by speeding up gazelle signifficantly. As
    @dougthor42 showed, the tree-sitter implementation is 3 times
    faster, which is a great performance win.
  • Re-implement a parser for the subset of the Python language that we
    care about in pure go.
  • Explain users how they can cross-compile the extension, but since
    there is no way to get the OSX sysroot on anything else than OSX os,
    this is not trivial to do.

None of the alternatives look appealing due to increased maintenance or
effort, hence the only real alternative for the time being is to revert.
If the cross compilation of CGO programs becomes better, then we can
look at reverting this revert and using tree-sitter in the future.

Fixes #1913

@aignas aignas requested review from f0rmiga and rickeylev as code owners May 30, 2024 04:06
@aignas aignas self-assigned this May 30, 2024
@dougthor42
Copy link
Contributor

Bummer! I really liked the pure golang helper, haha. Good thing Bazel 7.2.0rc1 is out and I can use git_override for rules_python_gazelle_plugin 🤣

I agree that most options are less than ideal. What does the 2nd option (re-implement) actually consist of? What are the risks?

@linzhp
Copy link
Contributor

linzhp commented May 30, 2024

We can potentially keep the part of using pypi/stdlib-list to resolve stdlib, which doesn't rely on C.

There are some Python parsers written in pure Go that we can potentially use. Not sure how good they are.

@aignas
Copy link
Collaborator Author

aignas commented May 31, 2024

Bummer! I really liked the pure golang helper, haha. Good thing Bazel 7.2.0rc1 is out and I can use git_override for rules_python_gazelle_plugin 🤣

I agree that most options are less than ideal. What does the 2nd option (re-implement) actually consist of? What are the risks?

@dougthor42, your comment suggests that merging this PR would be a mistake, because at least multiple people would use git_override then. :D

The reimplementation of the parser should not be too hard because we cane about a very small subset of the language:

  • comments
  • if __name__ == "__main__"
  • from foo import bar
  • import foo

The problem could be that Python is dynamic and it may have these statements within functions, which could means that we do need at least a decent parser to do that.

We can potentially keep the part of using pypi/stdlib-list to resolve stdlib, which doesn't rely on C.

There are some Python parsers written in pure Go that we can potentially use. Not sure how good they are.

@linzhp, Agree, having the pypi/sdlib-list to resolve stdlib is one of the nicest parts here. I haven't found any good parsers in go for Python, so if you could share the options, it would be great. Most of the things I was getting whilst googling were Go parsers in Python.

I'll make this PR a draft, but leave it open in case I hear good reasons for it to be merged. If we just put the cross-building of gazelle as something that our users need to think about themselves, then I am happy with having the CGo implementation of gazelle as the main version.

@aignas aignas marked this pull request as draft May 31, 2024 05:25
@linzhp
Copy link
Contributor

linzhp commented May 31, 2024

I haven't found any good parsers in go for Python

Looks like github.com/go-python/gpython/parser is a decent one

@aignas aignas marked this pull request as ready for review June 2, 2024 06:24
@aignas aignas closed this Jun 2, 2024
@dougthor42 dougthor42 mentioned this pull request Jun 2, 2024
Open
@aignas aignas mentioned this pull request Jun 4, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@f0rmiga f0rmiga Awaiting requested review from f0rmiga

@rickeylev rickeylev Awaiting requested review from rickeylev rickeylev is a code owner

Assignees

@aignas aignas

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot cross-compile Gazelle extension
3 participants
@aignas @dougthor42 @linzhp